Fix ERC777 potential reentrancy issues #2483
Merged
+50
−2
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
We received a report from @ritzdorf and @antonper about a potential security issue for people extending our ERC777 contract.
One of the characteristics of ERC777 is that it allows reentrancy through the send and receive hooks, so the token needs to be coded carefully to avoid a reentrancy attack. In particular, the contract needs to be in a consistent state whenever an external call is made to an untrusted address. For a detailed writeup about reentrancy check out our article Reentrancy After Istanbul.
The ERC777 contract we provide is safe against reentrancy, because during send and receive hooks it is in a consistent state. However, extending this contract with a custom
_beforeTokenTransfer
function could allow a reentrancy attack to happen. More specifically, when burning tokens,_beforeTokenTransfer
is invoked before the send hook is externally called on the sender while token balances are adjusted afterwards. At the moment of the call to the sender, which can result in reentrancy, state managed by_beforeTokenTransfer
may not correspond to the actual token balances or total supply.For example, in a hypothetical
ERC777Snapshot
contract with balance snapshots, a custom_beforeTokenTransfer
function may keep track of an account's balance history in a new mapping. When the send hook is called on the sender, the balance snapshot history and the current token balances would be out of sync, the kind of inconsistent state that is vulnerable to reentrancy attacks.Am I affected?
If you're using our implementation of ERC777 from version 3.3.0 or earlier, and you define a custom
_beforeTokenTransfer
function that writes to a storage variable, you may be vulnerable to a reentrancy attack. If you're affected and would like assistance please write to[email protected]
.PR Checklist